Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

added ibc-hooks #1173

Merged
merged 7 commits into from
Apr 15, 2024
Merged

added ibc-hooks #1173

merged 7 commits into from
Apr 15, 2024

Conversation

sampocs
Copy link
Collaborator

@sampocs sampocs commented Apr 12, 2024 •
edited
Loading

Context

Added IBCHooks by roughly following the guide here. The guide seemed to be in accurate in places, so I filled in the gaps by referencing osmosis/stargaze/neutron, and by leveraging my own understanding of the app.go wiring.

Note to reviewer: Please double check the ICS4Wrapper wiring.

Should we also restrict packets to be one of (a) autopilot, (b) pfm or (c) wasm?

Brief Changelog

  • Upgraded ibc to v7.4.0
  • Added IBCHooks module
  • Added store key

Testing

High Level Approach

  1. Deployed a test contract to stride on dockernet (ica oracle contract)
  2. Sent a transfer with a hook to add a metric to the oracle
  3. Confirmed that the state changed accordingly in the contract

Takeaways

I had to increase the relayer gas adjustment in order to get the tx relayed (since the OnRecv now has more computation). We'll want to update our relayers accordingly!

Steps

  • Started dockernet normally
>>> make start-docker
  • Uploaded the ICA oracle contract to stride
>>> strided tx wasm store ../ica-oracle/artifacts/ica_oracle.wasm --from val1 -y --gas 5000000
  • Determined the address that would execute the contract by creating a temporary unit test in the ibc-hooks repo and using their function that builds the "intermediate sender" address
senderBech32, err := ibchookskeeper.DeriveIntermediateSender(
	"channel-0",
	"cosmos1uk4ze0x4nvh4fk0xm4jdud58eqn4yxhrgl2scj", // gaia validator 1
	"stride",
)
fmt.Println(senderBech32)

Address: stride1038hhcqj3syag68jrzhj387uy0qx88u3ta537e4nk8xn8pwuxt5sl0xrr9
  • Instantiated the contract with the admin address as stride1038hhcqj3syag68jrzhj387uy0qx88u3ta537e4nk8xn8pwuxt5sl0xrr9 (the address that will be generated from ibc-hooks) since adding metrics to the oracle is permissioned
>>> init_msg='{"admin_address": "stride1038hhcqj3syag68jrzhj387uy0qx88u3ta537e4nk8xn8pwuxt5sl0xrr9", "transfer_channel_id": "channel-0"}'
>>> strided tx wasm instantiate 1 "$init_msg" --from val1 --label "ica-oracle" --no-admin --gas auto --gas-adjustment 1.3 -y

Contract Address: stride14hj2tavq8fpesdwxxcu44rty3hh90vhujrvcmstl4zr3txmfvw9szh70ur
  • Queried the oracle and confirmed there were no metrics
>>> strided q wasm contract-state smart stride14hj2tavq8fpesdwxxcu44rty3hh90vhujrvcmstl4zr3txmfvw9szh70ur '{"all_latest_metrics": {}}'

data:
  metrics: []
  • Sent a transfer from the hub to Stride with a wasm ibc hook to write a metric to the oracle
>>> attributes=$(echo '{"sttoken_denom": "stuatom"}' | base64)
>>> add_metric_message='{"post_metric": {"key": "stuatom_redemption_rate", "value": "1.1", "metric_type": "redemption_rate", "update_time": 100, "block_height": 1000, "attributes": "'$attributes'"}}'
>>> memo='{"wasm": {"contract": "stride14hj2tavq8fpesdwxxcu44rty3hh90vhujrvcmstl4zr3txmfvw9szh70ur", "msg": '"$add_metric_message"'}}'
>>> gaiad tx ibc-transfer transfer transfer channel-0 stride14hj2tavq8fpesdwxxcu44rty3hh90vhujrvcmstl4zr3txmfvw9szh70ur 1000uatom --from gval1 -y --memo "$memo" -y
  • Confirmed the metric was added
>>> strided q wasm contract-state smart stride14hj2tavq8fpesdwxxcu44rty3hh90vhujrvcmstl4zr3txmfvw9szh70ur '{"all_latest_metrics": {}}'

data:
  metrics:
  - attributes: eyJzdHRva2VuX2Rlbm9tIjogInN0dWF0b20ifQo=
    block_height: 1000
    key: stuatom_redemption_rate
    metric_type: redemption_rate
    update_time: 100
    value: "1.1"

@github-actions github-actions bot added C:app-wiring dependencies Pull requests that update a dependency file labels Apr 12, 2024
asalzmann
asalzmann reviewed Apr 12, 2024
View reviewed changes
app/app.go Show resolved Hide resolved
app/app.go Show resolved Hide resolved
asalzmann
asalzmann approved these changes Apr 12, 2024
View reviewed changes
Copy link
Contributor

@asalzmann asalzmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

shellvish
shellvish approved these changes Apr 15, 2024
View reviewed changes
Copy link
Contributor

@shellvish shellvish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great to me!

One caveat though that the wiring in app.go is a bit arcane, would just want to make sure we test this the migration thoroughly on localstride + testnet + locally. Might be worthwhile to make a list of tests that we want to do, and then we can verify on all of the various environments.

More than happy to help with that of course!

app/app.go Show resolved Hide resolved
app/app.go Show resolved Hide resolved
x/autopilot/types/parser.go Show resolved Hide resolved
@sampocs sampocs merged commit b27e712 into main Apr 15, 2024
10 checks passed
ethan-stride
ethan-stride reviewed Apr 16, 2024
View reviewed changes
Copy link
Contributor

@ethan-stride ethan-stride left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

x/autopilot/types/autopilot.go
type RawPacketMetadata struct {
Autopilot *struct {
Receiver string `json:"receiver"`
Stakeibc *StakeibcPacketMetadata `json:"stakeibc,omitempty"`
Claim *ClaimPacketMetadata `json:"claim,omitempty"`
} `json:"autopilot"`
Forward *interface{} `json:"forward"`
Wasm *interface{} `json:"wasm"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main thing I was confused about before was that normal packets could be picked up and interpreted by a wasm contract but the diagram you made in excalidraw of how all the middlewares interact helped a lot. I was confused because I thought before that only tx specifically aimed at cosmwasm modules would be seen by wasm but now I see how it all fits together and they use different parts of the data so that it is possible for these cosm middlewares and another middleware like autopilot can act on the same packets even using data from the same memo. Unit tests also made it clearer

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@asalzmann asalzmann asalzmann approved these changes

@shellvish shellvish shellvish approved these changes

@ethan-stride ethan-stride ethan-stride left review comments

@riley-stride riley-stride Awaiting requested review from riley-stride

Assignees
No one assigned
Labels
C:app-wiring dependencies Pull requests that update a dependency file
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@sampocs @asalzmann @shellvish @ethan-stride